Skip to content
This repository was archived by the owner on Jul 13, 2020. It is now read-only.

disable auto js extensions #219

Closed
wants to merge 2 commits into from
Closed

disable auto js extensions #219

wants to merge 2 commits into from

Conversation

guybedford
Copy link
Member

Here's a prototype for #218.

  • In normalize, we normalize relative to the parentName, and when none is provided we normalize relative to the baseURL.
  • Paths are applied in normalize, before normalization, only on names that do not start with "scheme:", "./", "../", "/".
  • Locate does a final normalization relative to the baseURL

All tests passing.

Feedback welcome @caridy.

@guybedford
Copy link
Member Author

Note most of this is tests. The only logic changes are in the system.js file at https://github.com/ModuleLoader/es6-module-loader/pull/219/files#diff-6387cea082f6e735c8c1d75728d615b6L127.

@caridy
Copy link
Contributor

caridy commented Sep 28, 2014

LGTM

// it is possible for locate to not to be a fully normalized URL
// if name was forced into not being of an absolute URL by normalize
// so we run toAbsoluteURL again just in case
return toAbsoluteURL(this.baseURL, load.name);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could throw here if load.name was not already absolute?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think we should throw, or just go with whatever we get blindly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this would mess up all uses of names like @system, @traceur. Or should these become schema names anyway?

@guybedford
Copy link
Member Author

Another question - in NodeJS, /Users/etc/.... is not a valid module name because it should be written - file:/Users/etc/... right?

@caridy
Copy link
Contributor

caridy commented Sep 30, 2014

no, I think / or C:\ are just find, they are absolute paths in the filesystem. Why are you asking?

@guybedford
Copy link
Member Author

Because / is still not a fully-normalized URI right, as in missing the schema?

Or is the absolute URI check just /^\/|:/?

@caridy
Copy link
Contributor

caridy commented Sep 30, 2014

well, my doc is only covering the browser side of it, ideally I will have time next week to work on the nodejs version of it, but I think on the nodejs runtime things are different, and the concept of URI is not longer valid since nodejs doesn't know about those.

@guybedford
Copy link
Member Author

Thanks... would be really appreciated. That's why I assumed a file: schema in Node like how browsers handle file paths.

@guybedford
Copy link
Member Author

The latest commit includes the throwing behaviour with the file: addition in Node (all tests passing).

@caridy
Copy link
Contributor

caridy commented Oct 1, 2014

LGTM

@guybedford
Copy link
Member Author

Great, thanks.

@@ -88,7 +88,7 @@ function runTests() {
var oldBaseURL = System.baseURL;
System.baseURL = 'http://example.org/a/b.html';

test('Normalize - No Referer', System.normalize('d/e/f'), 'd/e/f');
test('Normalize - No Referer', System.normalize('d/e/f'), 'http://example.org/a/d/e/f');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the idea was that if it doesn't start with './' etc then it should do no normalization?

Meaning, that the only way to load these would be to first register them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch @arv, that's indeed the idea. if there is no d/* pattern, it should throw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we expect all local paths to be loaded with ./?

Eg:

<script type="module">
  import "./app.js"
</script>

works by requesting relative to the baseURL, but app.js does not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this test should be:

test('Normalize - No Referer', System.normalize('d/e/f'), 'd/e/f');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was that paths would apply to these sorts of names to make them work, eg:

System.paths['d/*'] = 'http://site.com/path/*';

And if there was no path match, we can't fetch it?

We currently throw in locate if this was the case it has no schema - an alternative would be to move the throwing behaviour to fetch rather @caridy?

@@ -1,4 +1,4 @@
define(['./amd-dep'], function(dep) {
define(['./amd-dep.js'], function(dep) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the .js mandatory in AMD?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tough one actually - most AMD doesn't use extensions, so we could still have the AMD layer itself add the extension. In CommonJS we could do something similar assuming the extension is optional. I'm not sure about this though.

@guybedford
Copy link
Member Author

This has been replaced by #317.

@guybedford guybedford closed this Mar 3, 2015
@guybedford guybedford deleted the js branch April 2, 2015 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants